-
Notifications
You must be signed in to change notification settings - Fork 1.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
use writable directory for sqlite default location #3151
Conversation
/cc @michalsn |
Honestly speaking Overall I'm not sure if placing database in the Anyway, this change will stay with us for some time, so I can't be the one to decide about this. @lonnieezell, @MGatner any thoughts? |
I think that for permissions handling we should keep everything that the framework needs write access to in the writable folder. I understand the concerns from @ivantcholakov that this groups temp data together with important and sensitive data but I think the configuration and security issues are more important. Personally I always put my SQLite database at writable/database.db as it is super clear what it is (this is your database) and skips the extra folder. We also have precedence for this from the Playground: https://github.com/codeigniter4projects/playground/blob/develop/.env.example |
ok, closing it |
Well... now I'm wondering if placing database at writable/database.db isn't actually a better idea 😅 |
🤷♂️ I think it's open. Since #3142 hasn't been released I think it is safe to change, but I also think your solution is fine. The only additional advantage I see to writable/database.db is that it doesn't require a change to .gitignore and the new data folder, which amount to required updates to existing projects. |
It's definitely not too late for any changes in this matter. Another advantage is that when we access writable directory, we immediately see the database file, so it's less likely to be deleted by accident. |
Ok, I will update to point to writable dir |
Sounds good. Thanks you two! Good progress. It will be nice not having to set that variable in every SQLite test scenario now too :) |
Looking better! Do we know what |
updated with simplified check database !== 'memory' && strpos($this->database, DIRECTORY_SEPARATOR) === false for adding WRITEPATH prefix |
I would try to add |
I've added |
I updated tests config to use "database.db" to prove that the patch is working |
To avoid removed by incident as @ivantcholakov comment at #3142 (comment)Checklist: